Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport release-24.11] lockbook-desktop: 0.9.15 -> 0.9.16 #373822

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

nixpkgs-ci[bot]
Copy link
Contributor

@nixpkgs-ci nixpkgs-ci bot commented Jan 14, 2025

Bot-based backport to release-24.11, triggered by a label in #373059.

  • Before merging, ensure that this backport is acceptable for the release.
    • Even as a non-commiter, if you find that it is not acceptable, leave a comment.

(cherry picked from commit 6b5bf05)
@nixpkgs-ci nixpkgs-ci bot mentioned this pull request Jan 14, 2025
13 tasks
@wolfgangwalther
Copy link
Contributor

I doubt that you built or tested anything against the release-24.11 branch when approving so fast.

Maybe we should add the failed cherry-pick from #373058 here.

(cherry picked from commit 01aa1e3)
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 14, 2025
@Parth
Copy link
Member

Parth commented Jan 14, 2025

Open to advice on what I should be testing generally. Here is what's on my mind:

  • on lockbook's end we QA'd this release pretty thoroughly, but I imagine you're saying something nix specific may have broken?
  • I figured if it's some category of defect in our nix expression CI will catch the build breakage
  • If something is wrong in a dependency then that dependency, once fixed, would trigger a rebuild of our package.

We also release fairly often. But open to advice and updating my models generally.

Also wouldn't mind feedback on what the most nix-friendly way to delivery small updates often to our community is. I waited about 6 hours for @r-ryantm pickup our changes, but figured a pr like this may be faster. But I don't want to increase load on nix committers, and I'm not sure it actually is faster. So open to advice on that front as well.

@github-actions github-actions bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Jan 14, 2025
@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented Jan 14, 2025

Open to advice on what I should be testing generally.

Sure :)

  • on lockbook's end we QA'd this release pretty thoroughly, but I imagine you're saying something nix specific may have broken?

Yes, this is only about nix specific stuff. You never know - dependencies between master and release-24.11 might be different, so at least the build should be tested. I assume once it builds and you propose this as an upstream dev, I don't need to spend time running it or anything - you'll have taken care of that.

But we need to test it builds, so we're not breaking anyone updating.

  • I figured if it's some category of defect in our nix expression CI will catch the build breakage

Well, that's the point: nixpkgs CI does not build. Currently, at least.

  • If something is wrong in a dependency then that dependency, once fixed, would trigger a rebuild of our package.

Yes, but lockbook itself could also be broken for a specific dependency on a certain version. In that case, this would need investigation.

Also wouldn't mind feedback on what the most nix-friendly way to delivery small updates often to our community is. I waited about 6 hours for @r-ryantm pickup our changes, but figured a pr like this may be faster. But I don't want to increase load on nix committers, and I'm not sure it actually is faster. So open to advice on that front as well.

Yeah, not sure how fast r-ryantm is going to do that either. If you're opening a PR yourself, I think it's easier for both you and the reviewer to put lockbook and lockbook-desktop into one PR, two separate commits.

If you then run a nixpkgs-review on the PR (wait for CI's Eval to be through first, so you don't have to eval locally) and post the result, a potential reviewer might be ready to merge already. I would.

You could also add some labels like "approved by package maintainer" and "upstream developer", that would make the PR more discoverable and give reviewers a hint directly.

Also good would be to post a link to the release notes / changelog in the body of the PR, that helps reviewers check the changes for breaking changes when backporting etc.

I'm sure as an upstream developer + package maintainer you have all of those changes on your radar anyway, but it helps making the reviewer aware of that.

We also release fairly often.

Feel free to request review from me for your simple update PRs, ideally with the info above :)

(I like request review more than a ping by name in that case)

@wolfgangwalther
Copy link
Contributor

Feel free to request review from me for your simple update PRs, ideally with the info above :)

Addition: Ideally the request for review comes in when CI has run through already and the review result is there. With everything on the table, I could merge on the spot and not have to keep track of the PR myself.

@wolfgangwalther
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 373822


x86_64-linux

✅ 2 packages built:
  • lockbook
  • lockbook-desktop

aarch64-linux

✅ 2 packages built:
  • lockbook
  • lockbook-desktop

x86_64-darwin

✅ 1 package built:
  • lockbook

aarch64-darwin

✅ 1 package built:
  • lockbook

@wolfgangwalther wolfgangwalther merged commit 23a5299 into release-24.11 Jan 14, 2025
23 of 28 checks passed
@wolfgangwalther wolfgangwalther deleted the backport-373059-to-release-24.11 branch January 14, 2025 20:13
@Parth
Copy link
Member

Parth commented Jan 14, 2025

This was a very helpful message, thank you!

Thank you also for bringing over the failing cherry-pick.

I'll keep all of the above in mind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants